Skip to content

bench: default CI benches to ethrex 100tx continuations; widen rkyv pointers for >2 GiB proofs - #867

Open
ColoCarletti wants to merge 6 commits into
mainfrom
bench-ci-continuations
Open

bench: default CI benches to ethrex 100tx continuations; widen rkyv pointers for >2 GiB proofs#867
ColoCarletti wants to merge 6 commits into
mainfrom
bench-ci-continuations

Conversation

@ColoCarletti

@ColoCarletti ColoCarletti commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

What

  • /bench-gpu and /bench-abba now default to ethrex 100tx --continuations (the production proving mode). New syntax: /bench-gpu [N] [cont[TX]|mono[TX]]mono5 keeps the legacy monolithic workload.
  • rkyv now uses pointer_width_64: the default 32-bit offsets cap an archive at ~2 GiB, and the 100tx continuation proof is 3.13 GiB — proving worked, writing the file panicked. Proof size +2%, proving/verify time unchanged.

Breaking

Proof files serialized before this change can't be read by new binaries (and viceversa). RECURSION_INPUT_VERSION bumped 1→2 so stale recursion blobs are rejected cleanly. No proofs are persisted in-repo or in CI.

Validation (16-core + RTX 5090)

  • 100tx cont: GPU prove 203 s, CPU 410 s, proof 3.13 GiB, verify OK (30.7 s)
  • 10tx cont: 22.3 s (same as before the change), proof +2.04%

Notes

  • Comment-triggered workflows run from main, so this takes effect post-merge.
  • PR branches older than this fix must rebase before a cont bench ≥40tx; the config step now fails fast with a clear message instead of burning the build.

@ColoCarletti

Copy link
Copy Markdown
Collaborator Author

/ai-review

@github-actions

Copy link
Copy Markdown

Codex Code Review

  • Medium — Version the changed proof wire format. prover/Cargo.toml:30 changes every rkyv archive from 32-bit to 64-bit relative pointers. Existing monolithic proofs, continuation proofs, and recursion blobs become unreadable, but recursion blobs still advertise wire version 1 at prover/src/lib.rs:213, while CLI proof files have no format marker at all. Bump the recursion version and add/version a proof envelope—or retain a legacy decoding path—so incompatible artifacts are rejected explicitly and existing proofs remain verifiable where required.

@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review — bench workload defaults + rkyv pointer_width_64

Both halves look correct. The rkyv change is complete and the workflow arg parsing is well-hardened (digits-only validation before every [ -lt ], so dropping the old 2>/dev/null guards is safe; TX_COUNT/PAIRS are validated before being interpolated into the remote bash -lc). I verified scripts/bench_abba.sh on main already reads CONTINUATIONS/TX_COUNT, so the new env passing takes effect immediately after merge, and the tracked ethrex_10_transfers.bin matches the cont10 claim.

No Critical/High issues found. Findings:

Low/Medium — a stale PR branch burns hours before failing. With cont100 as the default, any PR side that predates the pointer_width_64 fix dies at proof-serialization time (>2 GiB archive), i.e. only after the ~30-40 min dual build and the first prove — ~1 hr of GPU rental, or a long block on the single bench server. The header comments say "must rebase", but nothing enforces it. A cheap pre-flight in the config step (when CONTINUATIONS=1 && TX_COUNT >= 40, check pointer_width_64 in the PR side's prover/Cargo.toml via the contents API and fail fast with "rebase past ") would turn a multi-hour failure into a 5-second one.

Low — pair-count ceiling vs. the new CPU timeout. See the inline note on timeout-minutes: 600: at the 40-pair clamp the run is within ~10-15 min of the timeout.

Low — breaking proof-file format + RECURSION_INPUT_VERSION. See the inline note on prover/Cargo.toml. Failure mode is a clean error, not UB, but it's worth documenting and the version constant is worth bumping.

Nit — for tok in $ARGS is unquoted, so tokens are glob-expanded (both workflows). /bench-gpu cont100 * expands against the runner's CWD — harmless today since unrecognized tokens only warn, but set -f before the loop (or set +f after) makes parsing independent of what happens to be in the workspace, which matters more on the persistent self-hosted bench runner.

Note, not a request — the ~40-line token-parse + clamp block is now duplicated across the two workflows with different defaults (20/40 vs 14/32 pairs). Factoring it out is awkward since the GPU job never checks out the repo, so duplication is probably the right call; just something to keep in sync.

Comment thread prover/Cargo.toml
@github-actions

Copy link
Copy Markdown

AI Review

PR #867 · 8 changed files

Findings

Status Sev Location Finding Found by
confirmed medium .github/workflows/bench-abba.yml:95 Inconsistent pair-count rounding between CPU and GPU ABBA workflows nemotron
openrouter/nvidia/nemotron-3-ultra-550b-a55b
moonmath
zro/minimax-m3
uncertain low .github/workflows/bench-abba.yml:35 bench-abba.yml timeout ceiling leaves minimal headroom at max pairs nemotron
openrouter/nvidia/nemotron-3-ultra-550b-a55b
confirmed low .github/workflows/benchmark-gpu.yml Stale '>=96GB RAM' in offer-failure error message inconsistent with the 48 GB filter glm
openrouter/z-ai/glm-5.2
kimi
openrouter/moonshotai/kimi-k2.7-code

Status column reflects the verdict from the verifier: deepseek-verifier (openrouter/deepseek/deepseek-v4-pro).

AI-001: Inconsistent pair-count rounding between CPU and GPU ABBA workflows
  • Status: confirmed
  • Severity: medium
  • Location: .github/workflows/bench-abba.yml:95
  • Found by: nemotron:openrouter/nvidia/nemotron-3-ultra-550b-a55b, moonmath:zro/minimax-m3
  • Verified by: deepseek-verifier:openrouter/deepseek/deepseek-v4-pro
  • Rejected by: -

Claim

bench-abba.yml clamps pair count to [2,40] but does not round odd values to even, while benchmark-gpu.yml does. Odd pair counts unbalance the AB/BA drift-cancellation pattern.

Evidence

In bench-abba.yml, after the diff: if [ "$PAIRS" -lt 2 ] || [ "$PAIRS" -gt 40 ]; then echo "::warning::pair count $PAIRS out of range [2,40]; using 20"; PAIRS=20; fi. There is no equivalent of the GPU's odd-pair rounding block (the diff added this block to benchmark-gpu.yml: if [ $((PAIRS % 2)) -ne 0 ]; then PAIRS=$((PAIRS + 1)); echo "::notice::rounded odd pair count up to $PAIRS so AB/BA orders balance"; fi). The bench_abba.sh script warns but does not error on odd pairs, so the unbalanced run proceeds.

Suggested fix

Add the same odd-pair rounding block from benchmark-gpu.yml (lines 144-147) to bench-abba.yml after the PAIRS clamp, so odd counts are silently rounded up to the next even number for both workflows.

AI-004: bench-abba.yml timeout ceiling leaves minimal headroom at max pairs
  • Status: uncertain
  • Severity: low
  • Location: .github/workflows/bench-abba.yml:35
  • Found by: nemotron:openrouter/nvidia/nemotron-3-ultra-550b-a55b
  • Verified by: -
  • Rejected by: -

Claim

600-minute timeout for 40 pairs of 100tx continuations (~7 min/prove = 560 min) leaves only ~40 min for dual builds and overhead. A slower runner or transient delay could hit the ceiling and kill the run.

Evidence

Workflow comment states 100tx-continuations prove is ~7 min on 16-core; 40 pairs = 80 proves = 560 min. Builds add ~10-20 min. Timeout is 600 min (line 35).

Suggested fix

Increase timeout to 660-720 minutes, or reduce max pairs clamp from 40 to 36 to restore headroom.

AI-006: Stale '>=96GB RAM' in offer-failure error message inconsistent with the 48 GB filter
  • Status: confirmed
  • Severity: low
  • Location: .github/workflows/benchmark-gpu.yml
  • Found by: glm:openrouter/z-ai/glm-5.2, kimi:openrouter/moonshotai/kimi-k2.7-code
  • Verified by: deepseek-verifier:openrouter/deepseek/deepseek-v4-pro
  • Rejected by: -

Claim

The offer-search error message still reports the old '>=96GB RAM' floor while the actual query filters cpu_ram>=48 and this PR's updated comment now documents 48 GB, so a failed offer search misleads the operator about the real constraint.

Evidence

The PR rewrites the comment above the QUERY to explain the 48 GB floor (continuations ~10 GB, legacy 5tx monolithic fits, widens dedicated pool ~15 vs ~11), but the echo '::error::No RTX 5090 offer matched ... >=96GB RAM ...' line below was left unchanged, so the user-visible failure text contradicts both the query (cpu_ram>=48) and the new comment.

Suggested fix

Update the error message to match the live filter and comment: "(... >=16 cores, >=48GB RAM, >=64GB disk, driver>=${MIN_DRIVER}, <= $${PRICE_CAP}/hr)".

Reviewer Lanes

Lane Model Prompt Status Findings
glm openrouter/z-ai/glm-5.2 general success 1
kimi openrouter/moonshotai/kimi-k2.7-code general success 1
minimax minimax/MiniMax-M3 general error: opencode failed (provider/auth/runtime error) and no findings were submitted 0
moonmath zro/minimax-m3 general success 3
nemotron openrouter/nvidia/nemotron-3-ultra-550b-a55b general success 4

Verification Lanes

Lane Model Status Confirmed Rejected Uncertain
deepseek-verifier openrouter/deepseek/deepseek-v4-pro success 2 4 1

Native Codex and Claude reviews run separately and post their own comments. They are not included in this structured provenance report.

Discarded candidates (4) — rejected by the verifier
  • Workflow doesn't pre-flight-check rkyv pointer_width_64 requirement before expensive build (.github/workflows/bench-abba.yml:9, found by moonmath:zro/minimax-m3) — The PR adds a comment documenting the pointer_width_64 requirement and the cont100 workload. Not adding a pre-flight check to inspect the PR's Cargo.toml is a missing feature, not a defect introduced by the PR. The comment is informational documentation about a known constraint — the PR doesn't make anything worse and the workflow runs as designed. This is too speculative to qualify as a real issue.
  • Default mono token silently ignores user-supplied TX_COUNT (last-token-wins can mask intent) (.github/workflows/bench-abba.yml:85, found by moonmath:zro/minimax-m3) — The for-loop/case token parser uses last-match-wins semantics, which is a deliberate design choice for a command-line parser. The comment on line 64 says tokens are processed 'in any order' — this implies later tokens overwrite earlier ones. A user typing contradictory tokens like 'cont100 mono5' gives contradictory instructions; the parser picking the last one is reasonable. The behavior is not a defect introduced by the PR but rather the designed semantics of the parser.
  • benchmark-gpu.yml workflow_dispatch mode parameter format not self-documenting (.github/workflows/benchmark-gpu.yml:18, found by nemotron:openrouter/nvidia/nemotron-3-ultra-550b-a55b) — The workflow_dispatch mode parameter description (line 27) says 'cont[TX]' where brackets denote optional — this is standard notation meaning the TX number is attached without a space. Someone typing 'cont 100' is using an incorrect format. The PR's parsing is consistent and the description is clear about the expected format. Flagging this as a defect is overly speculative.
  • rkyv version split between main workspace (0.8.10) and ethrex-fixtures (0.8.16) (tooling/ethrex-fixtures/Cargo.toml:21, found by nemotron:openrouter/nvidia/nemotron-3-ultra-550b-a55b) — The rkyv version split between the main workspace (0.8.10) and ethrex-fixtures (0.8.16) is pre-existing — the PR diff does not touch tooling/ethrex-fixtures/Cargo.toml at all. The PR adds pointer_width_64 to 0.8.10 dependencies across several crates, but this has no bearing on the separate ethrex-fixtures crate which uses 0.8.16 for a different purpose (generating ProgramInput fixtures, not proof serialization). The version drift is not introduced or exposed by this PR.

Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts.

@ColoCarletti ColoCarletti changed the title fix ci bench: default CI benches to ethrex 100tx continuations; widen rkyv pointers for >2 GiB proofs Jul 27, 2026

@MauroToscano MauroToscano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ AI-generated review. Produced by an automated agent and posted from this account — not written by hand. Findings below are leads to verify, not confirmed conclusions.

This PR turns on rkyv's pointer_width_64 across the proof-format crates, bumps RECURSION_INPUT_VERSION, and adds a CI pre-flight that blocks bench runs on branches predating the change. Both halves are internally consistent: the feature is on all six crates that declare rkyv, host and guest compile RECURSION_INPUT_VERSION from the same constant, every proof read stays on validated rkyv::access/from_bytes so old-format bytes yield an Err rather than a panic or unchecked access, the now-64-bit archived table dimensions remain gated by checked_mul plus the exact AIR-derived equality checks, and token parsing validates digits-only before every [ -lt ]. The findings below are workflow-side, plus one missing guard on the invariant the PR turns on.

.github/workflows/bench-abba.yml:102-112 (duplicated at benchmark-gpu.yml:135-146) — the pre-flight does not fail open on a failed fetch, contrary to its comment ("Skip if the fetch fails"). gh api prints HTTP error bodies to stdout, so on any 404/403/5xx SIDE holds a JSON error blob rather than the empty string: [ -n "$SIDE" ] passes, grep -q pointer_width_64 misses, and the job hard-exits with PR branch predates the rkyv pointer_width_64 fix without having read anything. Reproducible against this repo — gh api "repos/$GITHUB_REPOSITORY/contents/prover/Cargo.toml?ref=<bad-sha>" -H "Accept: application/vnd.github.raw" 2>/dev/null || true returns a {"message":"No commit found for the ref ...","status":"404"} body on stdout. A rate-limited token or a transient contents-API failure therefore turns the default cont100 bench into a hard block that blames the author for a stale branch. Fix: branch on gh's exit status (if SIDE=$(gh api ...); then ... fi) and/or require the payload to look like the file (e.g. it also contains rkyv) before treating a missing string as evidence the branch is stale. Secondary: the check is purely textual on one file's contents, so it also matches the feature name appearing only in the explanatory comment added above the dep; the whole check is deletable once every open branch postdates this merge.

.github/workflows/bench-abba.yml:38 — the Acknowledge step still runs before the cfg step (52-124), the reverse of benchmark-gpu.yml, which this PR restructured so config (71) precedes ack (163) and the notice interpolates steps.config.outputs.pairs/workload. Two effects. The notice is a hardcoded literal, so /bench-abba 4 cont10 still gets "~4-5 hr at the default 20 pairs of ethrex 100tx continuations; pass a smaller pair count or cont10 for a quicker run" — hedged by "at the default" so not false, but uninformative; pre-existing, widened by the new workload axis. New here: the fail-fast at line 110 fires after the "bench server is occupied until it finishes" comment, which — unlike the GPU workflow's marker-based update-in-place — is a plain createComment that is never retracted, and it is the default path (cont100TX_COUNT >= 40) for any un-rebased branch. The always() Post result step then posts a second comment headed `<sha>` vs `main` ( pairs, ethrex)pairs/workload are written at 118-123, after the exit 1, while head_sha lands at 63 before it — whose "Last log lines" block tails /tmp/abba_out.txt from a previous run, since the bench runner is persistent self-hosted and nothing cleans /tmp. Fix: move Acknowledge after cfg and interpolate steps.cfg.outputs.pairs/workload; the cfg step costs seconds, and the fail-fast then exits before any occupancy claim is posted. Optionally write the outputs block before the fail-fast (or rm -f /tmp/abba_out.txt at job start) so the failure comment is not half-blank with a stale log.

prover/Cargo.toml:27-29 — the pointer_width_64 invariant is held only by the "Keep in sync across all proof-format crates" comment plus five copies of it. Nothing in code or CI pins it: no const assert, no test, and RECURSION_INPUT_VERSION cannot help, since host and guest compile it from the same source (prover/src/lib.rs:214, checked at :265) — a rebuilt-but-misconfigured binary still advertises v2. rkyv compile_error!s on conflicting pointer-width features, but with none enabled it silently defaults to u32. Feature unification makes dropping it from a single main-workspace crate harmless; the two cases that bite are losing it from a whole graph (hoisting rkyv into [workspace.dependencies], or a version bump that drops the feature list) and the separate non-member bench_vs/lambda/recursion guest workspace drifting from the host. Both stay green in CI, since every existing test round-trips archives that fit 32-bit offsets, and only resurface as a serialization failure at the end of a >2 GiB prove. Mirror the const _: () = { assert!(...) } pattern already at prover/src/lib.rs:216-229: const _: () = assert!(size_of::<rkyv::primitive::ArchivedUsize>() == 8, "proof format requires rkyv pointer_width_64");. That compiles into both the host and the riscv64 guest graph (the guest path-depends on lambda-vm-prover, whose rkyv dep is non-optional), so it covers the drift the CI grep cannot see; placing it in crypto/math or crypto/stark under their rkyv feature would additionally cover standalone builds of the crates that define the archived types.

Nits:

  • prover/src/lib.rs:257 — no test covers the prefix rejection path (None on a short blob / wrong magic / wrong version, surfaced as Error::Execution("recursion blob: bad magic or version") at :286, continuation twin at recursion.rs:312); the three tests in prover/src/tests/recursion_smoke_test.rs (600, 649, 711) all leave the 12-byte prefix intact. Pre-existing (only the constant's value changes) and bounded to a less legible bytecheck Err, but worth closing since the breaking-change note leans on clean rejection and blobs are persisted (test_dump_recursion_input writes /tmp/recursion_input.bin, prebuilt recursion-*.elf guests are cached); recursion_archive_bytes is pure, so a test needs no proving — fabricate a 12-byte prefix plus dummy bytes and assert None for (a) version patched to 1u32 LE at bytes 4..8, (b) a flipped magic byte, (c) a blob shorter than RECURSION_INPUT_PREFIX_LEN.
  • prover/src/lib.rs:256 — the doc comment claiming an access_unchecked follows is stale; there is none in tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants